-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Add tracing support using Micrometer #1695
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm liking the approach.
Does this meet the spec for transactions and retryable operations?
I'm hoping so as both OperationContext#withSessionContext
and OperationContext#withTimeoutContext
reuse the operation id.
However, it would be worth clarifying before reviewing further.
de23d90
to
8e9d2b2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds comprehensive tracing support to the MongoDB Java driver using Micrometer as the tracing framework. The implementation provides tracing for commands, operations, and transactions with span hierarchies that allow for detailed observability of database interactions.
- Integration of Micrometer tracing with a new
MicrometerTracer
implementation - Command, operation, and transaction span creation with proper parent-child relationships
- Cursor tracking to link getMore operations with their originating queries
- Test infrastructure including unified test support and utility classes for span validation
Reviewed Changes
Copilot reviewed 45 out of 45 changed files in this pull request and generated 4 comments.
Show a summary per file
File | Description |
---|---|
gradle/libs.versions.toml | Adds Micrometer tracing dependencies and test bundles |
driver-core/src/main/com/mongodb/tracing/MicrometerTracer.java | Main tracer implementation that bridges MongoDB driver with Micrometer |
driver-core/src/main/com/mongodb/internal/tracing/TracingManager.java | Core tracing management with span lifecycle and context handling |
driver-core/src/main/com/mongodb/internal/connection/InternalStreamConnection.java | Command-level tracing integration with connection handling |
driver-sync/src/test/functional/com/mongodb/client/tracing/SpanTree.java | Test utility for validating span hierarchies and structures |
Multiple build.gradle.kts files | Dependency updates across driver modules |
Comments suppressed due to low confidence (1)
driver-core/src/test/unit/com/mongodb/internal/connection/CommandHelperTest.java:121
- [nitpick] Empty line removed unnecessarily. This appears to be an unrelated formatting change that should not be included in a tracing feature PR.
OperationContext createOperationContext() {
driver-core/src/main/com/mongodb/internal/tracing/package-info.java
Outdated
Show resolved
Hide resolved
notNull("fullName", fullName); | ||
this.fullName = fullName; | ||
this.databaseName = getDatatabaseNameFromFullName(fullName); | ||
this.databaseName = getDatabaseNameFromFullName(fullName); |
Copilot
AI
Jul 21, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Method name corrected from 'getDatatabaseNameFromFullName' to 'getDatabaseNameFromFullName' - this appears to be an unrelated typo fix that should not be included in a tracing feature PR.
Copilot uses AI. Check for mistakes.
private static void assertValid(final SpanNode reportedNode, final SpanNode expectedNode, | ||
final BiConsumer<BsonValue, BsonValue> valueMatcher) { | ||
// Check that the span names match | ||
if (!reportedNode.getName().equalsIgnoreCase(expectedNode.getName())) { |
Copilot
AI
Jul 21, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] Using case-insensitive comparison for span names may hide actual mismatches. Consider using exact string comparison unless case variations are explicitly expected.
if (!reportedNode.getName().equalsIgnoreCase(expectedNode.getName())) { | |
if (!reportedNode.getName().equals(expectedNode.getName())) { |
Copilot uses AI. Check for mistakes.
return false; | ||
} | ||
final SpanNode spanNode = (SpanNode) o; | ||
return name.equalsIgnoreCase(spanNode.name) |
Copilot
AI
Jul 21, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] Inconsistent case handling in equals method. The case-insensitive comparison should match the behavior used in assertValid method, but consider if this is the intended behavior.
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First review - overall looks really good, just a couple of nits.
My main point of contention is: Is an com.mongodb.internal.tracing
the correct place? Given that MongoClientSettings
accepts a TracingManager
that seems against the norm in the code base. Should there be a TracingManager
interface along with the other interfaces and then internal implementations of it? eg: TracingManagerImpl
What do you think?
* @return this | ||
* @since 5.5 | ||
*/ | ||
@Alpha(Reason.CLIENT) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Q: Is this going to be released as alpha?
driver-core/src/main/com/mongodb/internal/connection/CommandMessage.java
Outdated
Show resolved
Hide resolved
driver-core/src/main/com/mongodb/internal/connection/InternalStreamConnection.java
Outdated
Show resolved
Hide resolved
driver-core/src/main/com/mongodb/internal/connection/InternalStreamConnection.java
Outdated
Show resolved
Hide resolved
driver-core/src/main/com/mongodb/internal/connection/InternalStreamConnection.java
Outdated
Show resolved
Hide resolved
driver-core/src/main/com/mongodb/internal/operation/AggregateOperationImpl.java
Outdated
Show resolved
Hide resolved
driver-sync/src/main/com/mongodb/client/internal/ClientSessionImpl.java
Outdated
Show resolved
Hide resolved
driver-sync/src/test/functional/com/mongodb/client/tracing/ZipkinTracer.java
Outdated
Show resolved
Hide resolved
driver-sync/src/test/functional/com/mongodb/client/unified/Entities.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did a high level review of the API and the overall design.
driver-core/src/main/com/mongodb/internal/connection/CommandMessage.java
Outdated
Show resolved
Hide resolved
driver-core/src/main/com/mongodb/internal/tracing/TracingManager.java
Outdated
Show resolved
Hide resolved
import static java.lang.String.format; | ||
|
||
/** | ||
* Manages tracing spans for MongoDB driver activities. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not clear why a class like this is necessary. As I understand it, there is a single instance created at the scope of MongoClientSettings
, and treated as effectively a singleton by the MongoClient
instance. Would it be sufficient, and also simpler, to create a new instance of some class at the point where we start execution of an operation or transaction, and allow it to garbage collected when the operation or transaction completes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
New implementation uses a single instance per MongoClient.
private <T> T sendAndReceiveInternal(final CommandMessage message, final Decoder<T> decoder, | ||
final OperationContext operationContext) { | ||
CommandEventSender commandEventSender; | ||
Span tracingSpan = createTracingSpan(message, operationContext); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider pushing all the changes made to this class down into LoggingCommandEventSender
, which already contains a lot of similar logic. LoggingCommandEventSender
might need some refactoring as a result, but it seems better than messing too much with this class, which already has enough responsibilities as it is.
Paging @jonatan-ivanov |
Hi, I'm one of the Maintainers of Micrometer and Micrometer Tracing. I think the most important feedback I'd like to give is recommending to use the Observation API instead of "just" the Tracing API. The reason behind it is that this PR adds tracing capabilities but if you also want to add metrics support, this needs to be instrumented again. Also if users want (custom) logs or any custom output that they can figure out based on the instrumentation in these components, that also need to happen through a separate mechanism. Regarding to the Tracing API usage, I think the most important thing I would like to mention is that it is a Tracing Facade. It is what SLF4J is for loggers. As SLF4J can delegate to logging libraries, Micrometer Tracing can delegate to tracing libraries (Brave and OpenTelemetry right now but custom ones can be implemented by users). Because of this, it's usually not needed to pull another facade in front of it (just like it is usually not needed for SLF4J). Also, the Tracing and Observation APIs provide the features that are implemented here: The Observation API also has a nice feature that the Tracing API does not and it seems you could benefit from that (based on the |
Thanks, @jonatan-ivanov. That's very useful context. Seems like @mp911de commit to the Redis driver is close to what we would need to do here should we choose this route? Then configuration would be similar to this. |
Agree with #1695 (comment). I think this is the best way forward to get the most flexibility. |
…king with micrometer-tracing directly
@jyemin Sorry for the late response. |
FTR, you might want to ensure that your implementation isn't prone to the bug we fixed at spring-projects/spring-data-mongodb#5067 |
https://jira.mongodb.org/browse/JAVA-5733